feat: add macOS support to quickstart scripts#220
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughBoth setup and cleanup scripts now support a configurable container name (default ChangesCross-platform container configuration
Sequence Diagram(s)sequenceDiagram
participant HostShell as Host Shell
participant Podman as Podman CLI
participant PMachine as Podman Machine (VM)
participant LVM as LVM inside VM
HostShell->>Podman: prepare_lvm_disk (Darwin) -> podman machine ssh
Podman->>PMachine: establish SSH session
PMachine->>LVM: clear/create disk image, check/create VG
LVM-->>PMachine: VG state (exists/created)
PMachine-->>Podman: command exit/status
Podman-->>HostShell: return status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
agullon
force-pushed
the
macos-quickstart
branch
4 times, most recently
from
750c382 to
e1d0c51
Compare
|
@agullon , can we please also address the following issues in this PR?
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/quickstart.sh (2)
9-9: 💤 Low valueHeads up:
CONTAINER_NAMEconsumes$1underset -u.With
set -u, the${1:-microshift-okd}default is safe — but note any latershift-based arg parsing would now be off by one. Currently no other positional args, so it's fine as-is. Just leaving a breadcrumb for future args.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/quickstart.sh` at line 9, CONTAINER_NAME is assigned from positional $1 while the script uses set -u which will treat unset positional parameters as errors and any later use of shift would misalign args; to fix, either avoid consuming $1 directly by using a named option parsing or preserve positional parameters (e.g., parse args with getopts or copy $@ to another array) and update any shift logic accordingly, or explicitly document and guard CONTAINER_NAME assignment so future shift-based parsing isn’t off-by-one; refer to the CONTAINER_NAME assignment and any subsequent shift or positional parsing logic to implement one of these approaches.
63-85: 💤 Low valueDarwin reuse path doesn't early-return like the Linux branch.
On Linux (lines 87-91), when
lvm_diskexists you zero it andreturn 0immediately. On Darwin, after zeroing you fall through to thevgscheck and may re-runlosetup/vgcreate. Sinceddof 100MB likely wipes the LVM header,vgsreturns non-zero and a freshvgcreateruns — which is arguably the correct behavior for a "clear and reuse" cycle. Worth deciding whether Linux should match macOS (also recreate VG after zeroing) or vice versa, otherwise reuse semantics diverge subtly between platforms.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/quickstart.sh` around lines 63 - 85, The Darwin branch's heredoc handling of lvm_disk differs from Linux: after zeroing the existing lvm_disk with dd the Linux branch returns immediately (return 0) but the Darwin branch continues to run vgs/losetup/vgcreate, causing divergent reuse semantics; pick one behavior and make both branches consistent—either (A) match Linux by causing the Darwin heredoc to exit early after the dd (so the remote ssh block stops and the script returns 0) or (B) match Darwin by removing the immediate return in the Linux branch so both platforms proceed to vgs/losetup/vgcreate after zeroing; update the code around lvm_disk handling (the dd/truncate logic) and the flow control (return/exit) so lvm_disk and vg_name reuse is identical across platforms.src/quickclean.sh (1)
10-13: ⚡ Quick winRoot enforcement is inconsistent with
quickstart.shon macOS.
quickstart.sh(lines 169-195) lets Darwin run without root and only requires root on Linux. Here, root is required for every platform, so a user who quickstarted on macOS withoutsudocan't quickclean withoutsudo. Suggest mirroring the same Darwin/Linux split:♻️ Suggested mirror of quickstart's platform check
-# Check if the script is running as root -if [ "$(id -u)" -ne 0 ]; then - echo "ERROR: This script must be run as root (use sudo)" - exit 1 -fi +# Linux requires root; macOS uses the user's podman machine. +if [[ "$(uname -s)" != "Darwin" ]] && [ "$(id -u)" -ne 0 ]; then + echo "ERROR: This script must be run as root (use sudo)" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/quickclean.sh` around lines 10 - 13, The root-enforcement in quickclean.sh currently uses the id -u check unconditionally and should match quickstart.sh's platform-specific logic; change the check to only require root on non-Darwin platforms by detecting the OS via uname (e.g., test "$(uname -s)" = "Darwin") and skip the id -u root check when on Darwin, otherwise keep the existing id -u check and error message unchanged; update the conditional around the id -u test in quickclean.sh accordingly so macOS users who ran quickstart without sudo can run quickclean without sudo while Linux still requires root.README.md (1)
32-33: ⚡ Quick winMinimal macOS mention — consider a brief setup snippet.
The "including macOS" tag-on satisfies the literal ask, but new macOS users hitting this README still won't know they need a rootful Podman machine. Optional: add a short note (or link to a doc) with the four prerequisite commands from the PR description (
brew install podman,podman machine init --memory 4096,podman machine set --rootful,podman machine start) so the Bootc quickstart actually works on macOS out of the box.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 32 - 33, The README currently only mentions "including macOS" but lacks setup steps; add a short macOS prerequisite snippet near the sentence that mentions macOS (the paragraph starting "MicroShift Bootc container images can be run on `x86_64` and `aarch64`... including macOS") that shows the four required Podman commands (brew install podman, podman machine init --memory 4096, podman machine set --rootful, podman machine start) or a link to a macOS-specific doc, so macOS users know they must create a rootful Podman machine before following the quickstart.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/quickstart.sh`:
- Around line 70-85: The unquoted variable expansion ${machine_ssh} in
quickstart.sh triggers shellcheck SC2086 because it intentionally relies on
word-splitting; fix it either by adding a shellcheck suppression comment
immediately before the expansion (# shellcheck disable=SC2086) or by changing
machine_ssh to an array and invoking it as "${machine_ssh[@]}" where used;
locate the use of machine_ssh in the SSH command and apply one of these two
fixes and ensure any code that constructs machine_ssh is adjusted to produce an
array if you choose the array approach.
---
Nitpick comments:
In `@README.md`:
- Around line 32-33: The README currently only mentions "including macOS" but
lacks setup steps; add a short macOS prerequisite snippet near the sentence that
mentions macOS (the paragraph starting "MicroShift Bootc container images can be
run on `x86_64` and `aarch64`... including macOS") that shows the four required
Podman commands (brew install podman, podman machine init --memory 4096, podman
machine set --rootful, podman machine start) or a link to a macOS-specific doc,
so macOS users know they must create a rootful Podman machine before following
the quickstart.
In `@src/quickclean.sh`:
- Around line 10-13: The root-enforcement in quickclean.sh currently uses the id
-u check unconditionally and should match quickstart.sh's platform-specific
logic; change the check to only require root on non-Darwin platforms by
detecting the OS via uname (e.g., test "$(uname -s)" = "Darwin") and skip the id
-u root check when on Darwin, otherwise keep the existing id -u check and error
message unchanged; update the conditional around the id -u test in quickclean.sh
accordingly so macOS users who ran quickstart without sudo can run quickclean
without sudo while Linux still requires root.
In `@src/quickstart.sh`:
- Line 9: CONTAINER_NAME is assigned from positional $1 while the script uses
set -u which will treat unset positional parameters as errors and any later use
of shift would misalign args; to fix, either avoid consuming $1 directly by
using a named option parsing or preserve positional parameters (e.g., parse args
with getopts or copy $@ to another array) and update any shift logic
accordingly, or explicitly document and guard CONTAINER_NAME assignment so
future shift-based parsing isn’t off-by-one; refer to the CONTAINER_NAME
assignment and any subsequent shift or positional parsing logic to implement one
of these approaches.
- Around line 63-85: The Darwin branch's heredoc handling of lvm_disk differs
from Linux: after zeroing the existing lvm_disk with dd the Linux branch returns
immediately (return 0) but the Darwin branch continues to run
vgs/losetup/vgcreate, causing divergent reuse semantics; pick one behavior and
make both branches consistent—either (A) match Linux by causing the Darwin
heredoc to exit early after the dd (so the remote ssh block stops and the script
returns 0) or (B) match Darwin by removing the immediate return in the Linux
branch so both platforms proceed to vgs/losetup/vgcreate after zeroing; update
the code around lvm_disk handling (the dd/truncate logic) and the flow control
(return/exit) so lvm_disk and vg_name reuse is identical across platforms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b39a70c8-cdad-477a-aa7f-c54ffcd8d98b
📒 Files selected for processing (3)
README.mdsrc/quickclean.shsrc/quickstart.sh
agullon
force-pushed
the
macos-quickstart
branch
2 times, most recently
from
b37213d to
c50e834
Compare
quickstart.sh and quickclean.sh now detect macOS and handle podman machine validation (rootful mode required) and LVM setup via `podman machine ssh`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Usage: ./src/quickstart.sh [name] ./src/quickclean.sh [name] Defaults to 'microshift-okd' when no argument is provided. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/quickstart.sh (1)
165-187:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
${SUDO_USER}can be unbound when script is launched as actual root.With
set -u, if someone runs this directly asroot(not viasudo), the root check at line 165 happily passes, and then lines 172/181 (andprepare_lvm_diskat line 66) hit an unbound${SUDO_USER}and abort with a less-than-helpful error. The documented flow usessudo bash, so it usually works — but a small guard makes the failure mode much friendlier on macOS.🛡️ Suggested guard
# Platform-specific initialization if [[ "$(uname -s)" == "Darwin" ]]; then + if [ -z "${SUDO_USER:-}" ]; then + echo "ERROR: On macOS, run this script via sudo so SUDO_USER is set, e.g.:" + echo " curl -s https://${OWNER}.github.io/${REPO}/quickstart.sh | sudo bash" + exit 1 + fi if ! sudo -u "${SUDO_USER}" podman info &>/dev/null </dev/null; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/quickstart.sh` around lines 165 - 187, The script assumes SUDO_USER is set and will fail under set -u when run as actual root; add a guard after the root check to ensure SUDO_USER is defined (e.g. default it from logname or to "root" when empty) so references like ${SUDO_USER} in the podman checks and prepare_lvm_disk do not abort. Update quickstart.sh to set SUDO_USER="${SUDO_USER:-$(logname 2>/dev/null || echo root)}" (or similar) immediately after the if [ "$(id -u)" -ne 0 ] block so subsequent uses in the podman info/inspect checks and prepare_lvm_disk are safe.
🧹 Nitpick comments (1)
src/quickstart.sh (1)
63-95: 💤 Low valueBehavior parity note between Darwin and Linux branches.
Heads up (not a blocker): on Linux, when
lvm_diskalready exists youdd-zero it andreturn 0— skipping the VG check. On Darwin you zero and then still verify/recreate the VG. The Darwin behavior is arguably the more correct one (zeroing typically nukes LVM metadata), but the asymmetry will bite whoever debugs a stale VG on Linux next. Worth aligning in a follow-up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/quickstart.sh` around lines 63 - 95, When lvm_disk exists on Linux the code zeros it and returns early, skipping the VG existence check, which differs from the Darwin branch; remove the early "return 0" and follow the same flow as the Darwin branch: after dd-zeroing the existing "${lvm_disk}" check for the volume group (using "vgs '${vg_name}'" or similar) and if missing create it via losetup + "vgcreate -f -y '${vg_name}' \"${DEVICE}\"" (or reuse the common VG creation logic), ensuring the Linux branch uses the same vgexist/create sequence as the Darwin branch and referencing lvm_disk, vg_name, losetup, and vgcreate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/quickstart.sh`:
- Around line 165-187: The script assumes SUDO_USER is set and will fail under
set -u when run as actual root; add a guard after the root check to ensure
SUDO_USER is defined (e.g. default it from logname or to "root" when empty) so
references like ${SUDO_USER} in the podman checks and prepare_lvm_disk do not
abort. Update quickstart.sh to set SUDO_USER="${SUDO_USER:-$(logname 2>/dev/null
|| echo root)}" (or similar) immediately after the if [ "$(id -u)" -ne 0 ] block
so subsequent uses in the podman info/inspect checks and prepare_lvm_disk are
safe.
---
Nitpick comments:
In `@src/quickstart.sh`:
- Around line 63-95: When lvm_disk exists on Linux the code zeros it and returns
early, skipping the VG existence check, which differs from the Darwin branch;
remove the early "return 0" and follow the same flow as the Darwin branch: after
dd-zeroing the existing "${lvm_disk}" check for the volume group (using "vgs
'${vg_name}'" or similar) and if missing create it via losetup + "vgcreate -f -y
'${vg_name}' \"${DEVICE}\"" (or reuse the common VG creation logic), ensuring
the Linux branch uses the same vgexist/create sequence as the Darwin branch and
referencing lvm_disk, vg_name, losetup, and vgcreate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2582d6e-cd9c-4a48-9827-b91291c85d74
📒 Files selected for processing (3)
README.mdsrc/quickclean.shsrc/quickstart.sh
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/quickclean.sh
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
2 participants
Adds macOS support to
quickstart.shandquickclean.sh. The same scripts now work on both Linux and macOS.On macOS, the scripts require a rootful podman machine:
Changes:
quickstart.sh: detect macOS, validate rootful podman machine, run LVM setup inside the VM viapodman machine sshquickclean.sh: detect macOS, clean up LVM inside the VM viapodman machine ssh./src/quickstart.sh [name]🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation
Closes #222